Fw 956 refactor on changes #28187

benlesh · 2019-01-16T17:39:48Z

This change inverts the control over who is setting up onChanges to fire from the parent template to the component/directive definition itself. The idea is that the new NgOnChangesFeature will add a setInput function to the directive definition that is to be called when setting an input from a parent template. That setInput function will be composed to update a SimpleChanges object that is now stored at a known private property on the component instance.

Reverts most of the changes from the #27965 PR that moved control of onChanges into the parent template's instructions.

mary-poppins · 2019-01-16T17:43:50Z

You can preview 9cff51c at https://pr28187-9cff51c.ngbuilds.io/.

mary-poppins · 2019-01-16T17:56:25Z

You can preview c39d3c7 at https://pr28187-c39d3c7.ngbuilds.io/.

kara

General approach looks good, but we need the tests turned on to verify

packages/upgrade/test/static/integration/downgrade_module_spec.ts

packages/upgrade/test/static/integration/downgrade_component_spec.ts

packages/upgrade/test/dynamic/upgrade_spec.ts

packages/core/test/render3/ng_on_changes_feature_spec.ts

kara · 2019-01-16T19:43:44Z

packages/core/test/bundling/hello_world/bundle.golden_symbols.json

@@ -81,13 +81,13 @@
    "name": "NO_PARENT_INJECTOR"
  },
  {
-    "name": "NodeInjectorFactory"
+    "name": "NgOnChangesFeature"


It seems odd that we are bringing NgOnChangesFeature into hello_world. Why would that be the case?

Suspect this is a bug with the how the compiler is identifying the presence of the feature and adding it. I think we can fix this in a separate PR.

I would prefer we get to the bottom of extra inclusion, unless it is blocking something. Fix up PR later tend to not happen.

packages/compiler/src/render3/view/api.ts

packages/core/src/interface/lifecycle_hooks.ts

JoostK

Yeah! 🎉

JoostK · 2019-01-16T19:45:02Z

packages/core/src/render3/instructions.ts

+    ngDevMode && assertDataInRange(lView, index);
+    const def = tView.data[index] as DirectiveDef<any>;
+    const setInput = def.setInput;
+    if (setInput) {


The intermediate assignment seems unnecessary, removing it avoids the !:

if (def.setInput) { def.setInput(instance, value, publicName, privateName); }

It's an optimization to prevent it from being looked up twice.

@benlesh it would be, if the call didn't rely on this being the DirectiveDef (or we should use setInput.call(def, …). We are currently reading it twice already because of that.

JoostK · 2019-01-16T19:54:10Z

packages/core/src/render3/features/ng_onchanges_feature.ts

+function ngOnChangesSetInput<T>(
+    this: DirectiveDef<T>, instance: T, value: any, publicName: string, privateName: string): void {
+  const simpleChangesStore = getSimpleChangesStore(instance) ||
+      setSimpleChangesStore(instance, {previous: null, current: null});


Seems as if we can assign EMPTY_OBJ into previous here immediately, such that it is guaranteed to be present (the NgSimpleChangesStore.previous property may loose null from its type and the assignment of local var previous below can be inlined into the assignment of previousChange)

mary-poppins · 2019-01-17T00:31:06Z

You can preview 8c07bd4 at https://pr28187-8c07bd4.ngbuilds.io/.

mary-poppins · 2019-01-17T00:42:01Z

You can preview 00bc640 at https://pr28187-00bc640.ngbuilds.io/.

kara

LGTM.

ngbot · 2019-01-18T02:00:59Z

I see that you just added the PR action: merge label, but the following checks are still failing:
status "ci/circleci: integration_test" is failing
status "google3" is pending

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

benlesh · 2019-01-18T02:05:39Z

presubmit: http://test/OCL:229648942:BASE:229853217:1547777095425:d64118f5

alxhub · 2019-01-18T17:31:16Z

Note: missing reviews.

benlesh · 2019-01-18T17:51:45Z

packages/common/src/directives/ng_template_outlet.ts

@@ -34,20 +34,14 @@ import {Directive, EmbeddedViewRef, Input, OnChanges, SimpleChange, SimpleChange
 */
 @Directive({selector: '[ngTemplateOutlet]'})
 export class NgTemplateOutlet implements OnChanges {
-  private _viewRef: EmbeddedViewRef<any>|null = null;
+  // TODO(issue/24571): remove '!'.


This file appears to have been accidentally reverted when I reverted the other onChanges changes.

(changes changes changes)

- Wraps the NgOnChangesFeature in a factory such that no side effects occur in the module root - Adds comments to ngInherit property on feature definition interface to help guide others not to make the same mistake - Updates compiler to generate the feature properly after the change to it being a factory - Updates appropriate tests

benlesh · 2019-01-22T19:24:44Z

Adding one more bit of work to this to resolve FW-965, which is that the NgOnChangesFeature, even as it previously existed, was being included in the hello_world bundle, which makes no reference to it.

benlesh · 2019-01-22T19:31:39Z

presubmit: http://test/OCL:229648942:BASE:230378383:1548185415508:de230301

kara

LGTM

kara · 2019-01-22T19:50:45Z

packages/core/src/render3/interfaces/definition.ts

+   *
+   * NOTE: DO NOT SET IN ROOT OF MODULE! Doing so will result in tree-shakers/bundlers
+   * identifying the change as a side effect, and the feature will be included in
+   * every bundle.


Thanks for including this 👍

kara · 2019-01-22T19:57:14Z

merge assistance: no longer need public-API approval because the public API changes were reverted (no longer in this PR). Should be good with approval from @mhevery and i.

PR Close #28187

- Wraps the NgOnChangesFeature in a factory such that no side effects occur in the module root - Adds comments to ngInherit property on feature definition interface to help guide others not to make the same mistake - Updates compiler to generate the feature properly after the change to it being a factory - Updates appropriate tests PR Close #28187

- adds fixmeIvy annotation to tests that should remain updated so we can resolve those issues in the subsequent commits PR Close angular#28187

PR Close angular#28187

…ar#28187) - Wraps the NgOnChangesFeature in a factory such that no side effects occur in the module root - Adds comments to ngInherit property on feature definition interface to help guide others not to make the same mistake - Updates compiler to generate the feature properly after the change to it being a factory - Updates appropriate tests PR Close angular#28187

angular-automatic-lock-bot · 2019-09-14T14:52:21Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

benlesh requested review from a team as code owners January 16, 2019 17:39

googlebot added the cla: yes label Jan 16, 2019

benlesh added target: major This PR is targeted for the next major release comp: ivy risk: low labels Jan 16, 2019

ngbot bot added this to the needsTriage milestone Jan 16, 2019

benlesh force-pushed the FW-956_refactor-onChanges branch from 9cff51c to c39d3c7 Compare January 16, 2019 17:51

kara requested changes Jan 16, 2019

View reviewed changes

kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 16, 2019

kara assigned benlesh Jan 16, 2019

JoostK reviewed Jan 16, 2019

View reviewed changes

kara approved these changes Jan 17, 2019

View reviewed changes

kara removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 17, 2019

kara unassigned benlesh Jan 17, 2019

benlesh added the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jan 18, 2019

ngbot bot added the action: merge The PR is ready for merge by the caretaker label Jan 18, 2019

marclaval mentioned this pull request Jan 18, 2019

Lifecycle recursion #28239

Closed

benlesh removed the cla: yes label Jan 18, 2019

googlebot added the cla: yes label Jan 18, 2019

benlesh commented Jan 18, 2019

View reviewed changes

benlesh added 5 commits January 18, 2019 12:44

refactor(ivy): Add newer, smaller NgOnChangesFeature

bc5e5ae

fixup! refactor(ivy): Add newer, smaller NgOnChangesFeature

60840ef

fixup! refactor(ivy): Add newer, smaller NgOnChangesFeature

17b1d5a

fixup! refactor(ivy): Add newer, smaller NgOnChangesFeature

87018e9

fixup! refactor(ivy): Add newer, smaller NgOnChangesFeature

907b92f

benlesh force-pushed the FW-956_refactor-onChanges branch from 99787db to 907b92f Compare January 18, 2019 20:48

fixup! refactor(ivy): Add newer, smaller NgOnChangesFeature

75d970a

alxhub added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Jan 22, 2019

benlesh added state: WIP and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jan 22, 2019

fixup! fix(ivy): NgOnChangesFeature no longer included in hello_world

8853530

kara approved these changes Jan 22, 2019

View reviewed changes

mhevery approved these changes Jan 22, 2019

View reviewed changes

benlesh added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed state: WIP labels Jan 22, 2019

alxhub closed this in 5552661 Jan 23, 2019

alxhub pushed a commit that referenced this pull request Jan 23, 2019

refactor(ivy): Add newer, smaller NgOnChangesFeature (#28187)

a95e819

PR Close #28187

vetom pushed a commit to vetom/angular that referenced this pull request Jan 31, 2019

refactor(ivy): Add newer, smaller NgOnChangesFeature (angular#28187)

9460d92

PR Close angular#28187

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fw 956 refactor on changes #28187

Fw 956 refactor on changes #28187

benlesh commented Jan 16, 2019

mary-poppins commented Jan 16, 2019

mary-poppins commented Jan 16, 2019

kara left a comment

kara Jan 16, 2019

benlesh Jan 17, 2019

mhevery Jan 17, 2019

JoostK left a comment

JoostK Jan 16, 2019

benlesh Jan 17, 2019

JoostK Jan 17, 2019

JoostK Jan 16, 2019

mary-poppins commented Jan 17, 2019

mary-poppins commented Jan 17, 2019

kara left a comment

ngbot bot commented Jan 18, 2019

benlesh commented Jan 18, 2019

alxhub commented Jan 18, 2019

benlesh Jan 18, 2019

benlesh commented Jan 22, 2019

benlesh commented Jan 22, 2019

kara left a comment

kara Jan 22, 2019

kara commented Jan 22, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019

Fw 956 refactor on changes #28187

Fw 956 refactor on changes #28187

Conversation

benlesh commented Jan 16, 2019

mary-poppins commented Jan 16, 2019

mary-poppins commented Jan 16, 2019

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Jan 17, 2019

mary-poppins commented Jan 17, 2019

kara left a comment

Choose a reason for hiding this comment

ngbot bot commented Jan 18, 2019

benlesh commented Jan 18, 2019

alxhub commented Jan 18, 2019

Choose a reason for hiding this comment

benlesh commented Jan 22, 2019

benlesh commented Jan 22, 2019

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kara commented Jan 22, 2019

angular-automatic-lock-bot bot commented Sep 14, 2019